Added ability to include recipes smartly #1765

sjamgade · 2019-01-09T12:12:02Z

Roles can get smart by making recipe inform the role about its dependencies
across the proposal.

So a recipe will be included by the role only if the attributes
registered by the recipe are actually changed in current chef-client
run.

a recipe can register its dependencies as

mydependson = {
    "horizon::server" => [
        ['glance','api','bind_port']
    ]
}

BarclampLibrary::Barclamp::DependsOn.add(mydependson)

So here the "horizon::server" is informing about its
dependencies.

This is accomplished by comparing the value of the current attributes of node
against the one already stored in the databag after the previous successful
chef-client run.

BarclampLibrary::Barclamp::DependsOn.add:
takes in a map:

     recipe_name => [
                [barclampname,drill,down,till,value],
                [otherbarclampname,onlyhere],
               ]

This map is flushed and recreated only everyrun, however like resources
this could also be cached.

this behaviour can be altered by adding flag to config and using it
'include_recipe_smartly', however that is an extension to this behavior
and can be addressed in subsequent commits

chef/cookbooks/barclamp/libraries/smartroles.rb

chef/cookbooks/barclamp/libraries/barclamp_library.rb

hound · 2019-01-09T12:12:58Z

chef/cookbooks/barclamp/libraries/barclamp_library.rb

+            @recipe_depedancies ||= Mash.new
+            @recipe_depedancies.merge!(dependency)
+        end
+        def get(recipe)


Layout/EmptyLineBetweenDefs: Use empty lines between method definitions. (https://github.com/bbatsov/ruby-style-guide#empty-lines-between-methods)

chef/cookbooks/barclamp/libraries/barclamp_library.rb

nicolasbock

Can you think of a way to test this feature? Replacing include_recipe would introduce a potentially large regression 😄

Can you give some ballpark as to what performance impact this would have? Or maybe do you have examples where you measured the performance impact?

chef/cookbooks/barclamp/libraries/barclamp_library.rb

sjamgade · 2019-01-09T13:24:53Z

i did do some initial testing, here is the sample: sjamgade/crowbar-openstack@4fc8662

this is not going to replace include_recipe. but add alternate way to conditionally execute recipes. These both can coexist

nicolasbock · 2019-01-09T14:05:10Z

What I was trying to get at was that by replacing an include_recipe with an include_recipe_smartly in a recipe you are expecting a performance gain because the dependency is not included.

Besides testing whether a dependency is really not included when its attributes have not changed, we need to ask the following question:

For maximizing performance gain, the list of attributes of a dependency needs to be minimal in a sense that it contains only necessary attributes. However, a missing attribute introduces a regression.

How do we test that this set does not introduce a regression because it's missing something?

sjamgade · 2019-01-09T14:26:37Z

thats one way to look at it.
I see it other way: where recipes (and the resources) are grouped based on attributes.

one shortcut I though of
eg:

mydependson = {
    "horizon::server" => [
        ["horizon"]       # depend on the entire proposal
        ['glance','api','bind_port']    # this is across barclamps, 
    ]
}

I do no expect to see too many across barclamp dependencies and if there are, these recipes (and/or resources ) can be grouped a separate recipe

chef/cookbooks/barclamp/libraries/smartroles.rb

chef/cookbooks/barclamp/libraries/barclamp_library.rb

chef/cookbooks/barclamp/libraries/smartroles.rb

chef/cookbooks/barclamp/libraries/barclamp_library.rb

hound · 2019-01-22T14:35:45Z

chef/cookbooks/barclamp/libraries/barclamp_library.rb

+          prev_cfg = load(group, barclamp, instance)
+          instance = infer_instance(group, barclamp, instance)
+          cur_cfg = Chef::Role.load "#{barclamp}-config-#{instance}"
+          return prev_cfg, cur_cfg.default_attributes[barclamp]


Style/RedundantReturn: Redundant return detected. To return multiple values, use an array. (https://github.com/bbatsov/ruby-style-guide#no-explicit-return)

hound · 2019-01-22T15:24:57Z

chef/cookbooks/barclamp/libraries/smartroles.rb

+            Chef::Log.debug("[smart] due to change in: #{dependency}")
+            include_recipe recipe
+            return
+          end # each


Style/CommentedKeyword: Do not place comments on the same line as the end keyword.

chef/cookbooks/barclamp/libraries/smartroles.rb

skazi0 · 2019-01-24T10:08:02Z

chef/cookbooks/barclamp/libraries/barclamp_library.rb

+              return hash
+            end
+          end
+          Chef::Log.debug("[smart] hash does not respond to []")


this (and the variable name) is misleading. if variable doesn't respond to [] it's most probably not a hash.

how about collection?
since it is a collection of values

it could also be a single value. this is exactly when the log would trigger. I generally think that this message could be more informative (e.g. "%param name% is not a collection" or sth like this).

exactly, it could be a single value or a collection ( i could not think of a word for that)
and that is why the log says the collection did not respond to ... because it is a int/string/....

chef/cookbooks/barclamp/libraries/barclamp_library.rb

Roles can get smart by making recipe inform the role about its dependencies across the proposal. So a recipe will be included by the role only if the attributes registered by the recipe are actually changed in current chef-client run. a recipe can register its dependencies as ``` mydependson = { "horizon::server" => [ ['glance','api','bind_port'] ] } BarclampLibrary::Barclamp::DependsOn.add(mydependson) ``` So here the "horizon::server" is informing about its dependencies. This is accomplished by comparing the value of the current attributes of node against the one already stored in the databag after the previous successful chef-client run. BarclampLibrary::Barclamp::DependsOn.add: takes in a map: recipe_name => [ [barclampname,drill,down,till,value], [otherbarclampname,onlyhere], ] This map is flushed and recreated only everyrun, however like resources this could also be cached. this behaviour can be altered by adding flag to config and using it 'include_recipe_smartly', however that is an extension to this behavior and can be addressed in subsequent commits Use the role(proposal) that was committed to compare against databag This object enables us to do real comparison on proposal level. Thus every barclamp can have proposal level dependency hound fixes and refactor

hound · 2019-01-24T13:47:29Z

chef/cookbooks/barclamp/libraries/barclamp_library.rb

+              collection = collection[item]
+            end
+          rescue NoMethodError
+            Chef::Log.info("[smart] collection did not respond to []/key? while looking for #{item}")


Metrics/LineLength: Line is too long. [101/100] (https://github.com/SUSE/style-guides/blob/master/Ruby.md#metricslinelength)

hound · 2019-01-24T13:47:30Z

chef/cookbooks/barclamp/libraries/barclamp_library.rb

+              collection = collection[item]
+            end
+          rescue NoMethodError
+            Chef::Log.info("[smart] collection did not respond to []/key? while looking for #{item}")


Metrics/LineLength: Line is too long. [101/100] (https://github.com/SUSE/style-guides/blob/master/Ruby.md#metricslinelength)

sjamgade · 2019-01-24T16:46:31Z

Modified the example test to include proposal level depedancy : sjamgade/crowbar-openstack@954354e

vuntz · 2019-01-24T20:17:25Z

One key thing to be aware of is that not all barclamps save something to the data bags, and among the ones that do, not all of them simply copy the content of the proposal (eg: dns and ntp).

So the proposed approach here only works for the openstack barclamps. Which is still not too bad, but still something to be aware of.

sjamgade · 2019-01-28T15:06:38Z

I didnot know that, thanks @vuntz
Non-openstack barclamps are relatively fast and I dont think we need to consider them with respect to this PR.

any more reviews ? @skazi0

rhafer

Non-openstack barclamps are relatively fast and I dont think we need to consider them with respect to this PR.

But I guess it means that if e.g. something (anything) changes in a barclamp that doesn't maintain the config databags in the way we expect it, we need to always run the full set of recipes, right? As far as I can see the current code doesn't really handle that case. Also keep in mind that even not all barclamps from crowbar-openstack are writing the config databags yet. At least database is not using it currently I think.

I haven't fully made up my mind on this change yet. On the one hand it introduces a bit of magic that can have weird side-effects (e.g. a local chef-client run will not revert up any manual config change that were done or crowbar_join might be missing stuff on boot up). And there is a risk of this change causing some "interesting" corner cases. OTOH, I guess this will reduce the time needed for a deployment and may be more importantly for later config changes so there is definitely a significant benefit.

In general I think the code could benefit from a few more comments here and there to make it easier for reviewers to understand what it actually does.

mydependson = {
    "horizon::server" => [
        ["horizon"]       # depend on the entire proposal

Does this part really work with the current code?

BTW, I didn't really check this, but are you sure that this PR also works when e.g. the chef-client run on a node fails during apply. (Is the databag only written after a successful run?)

chef/cookbooks/barclamp/libraries/barclamp_library.rb

sjamgade · 2019-01-31T11:48:58Z

Non-openstack barclamps are relatively fast and I dont think we need to consider them with respect to this PR.

But I guess it means that if e.g. something (anything) changes in a barclamp that doesn't maintain the config databags in the way we expect it, we need to always run the full set of recipes, right? As far as I can see the current code doesn't really handle that case. Also keep in mind that even not all barclamps from crowbar-openstack are writing the config databags yet. At least database is not using it currently I think.

you are right, the code does not handle this case (recipes dependent on stuff outside the databags), but such recipes can continue to use include_recipe and only those which rely entirely on databag can use include_recipe_smartly
This PR just adds support for something like this and does not alter the current behavior.

I haven't fully made up my mind on this change yet. On the one hand it introduces a bit of magic that can have weird side-effects (e.g. a local chef-client run will not revert up any manual config change that were done or crowbar_join might be missing stuff on boot up). <snip>

local chef run can be asked to always run via a "config" option, which I will add if this PR sees the master branch.

In general I think the code could benefit from a few more comments here and there to make it easier for reviewers to understand what it actually does.
Will do. Thanks.
mydependson = {
    "horizon::server" => [
        ["horizon"]       # depend on the entire proposal
Does this part really work with the current code?

yes I have been testing it. :-)

BTW, I didn't really check this, but are you sure that this PR also works when e.g. the chef-client run on a node fails during apply. (Is the databag only written after a successful run?)

yes :-)

sjamgade · 2019-02-08T08:33:49Z

I understand there is a lot of uncertainty about the behavior of almost all barclamps as everyone here is suggesting.

But this PR is merely adding support for skipping unnecessary stuff and not actually affecting the real workflow. And as followup PR, I will add the option to disable skipping entirely.

Is there anything else we can do to move things forward here.

rhafer · 2019-02-08T14:26:52Z

I understand there is a lot of uncertainty about the behavior of almost all barclamps as everyone here is suggesting.

There is also some uncertainty (at least from me) about how much additional maintenance this requires. This finally adds another place where we carefully need to think and maintain dependencies between barclamps (at least of the barclamps for which we actually want to use it).

We haven't been particularly good at that in the past IMO :)
You're obviously right, though that this PR itself doesn't change any behavior for now until we start adapting the role_recipes. Maybe it's fine to just merge it and continue with experitentally enabling some roles for this.

Is there anything else we can do to move things forward here.

I guess some more reviews/opinions form others might help. @aspiers maybe?

most concerns I raised have been addressed

hound · 2019-02-25T13:05:32Z

chef/cookbooks/barclamp/libraries/smartroles.rb

+            include_recipe recipe
+            included = true
+            break
+          end.empty? and begin


Style/AndOr: Use && instead of and. (https://github.com/bbatsov/ruby-style-guide#no-and-or-or)

hound · 2019-02-25T13:05:32Z

chef/cookbooks/barclamp/libraries/smartroles.rb

+		#    "horizon::server" => [
+		#        ['glance','api','bind_port'],
+		#        ['horizon']
+		#    ]


Layout/Tab: Tab detected. (https://github.com/bbatsov/ruby-style-guide#spaces-indentation)
Layout/CommentIndentation: Incorrect indentation detected (column 2 instead of 8).

hound · 2019-02-25T13:05:32Z

chef/cookbooks/barclamp/libraries/smartroles.rb

+        # if it has changed include the recipe
+		#    "horizon::server" => [
+		#        ['glance','api','bind_port'],
+		#        ['horizon']


Layout/Tab: Tab detected. (https://github.com/bbatsov/ruby-style-guide#spaces-indentation)

hound · 2019-02-25T13:05:32Z

chef/cookbooks/barclamp/libraries/smartroles.rb

+		# Iterate over the dependcies and  find if either have changed
+        # if it has changed include the recipe
+		#    "horizon::server" => [
+		#        ['glance','api','bind_port'],


Layout/Tab: Tab detected. (https://github.com/bbatsov/ruby-style-guide#spaces-indentation)

hound · 2019-02-25T13:05:32Z

chef/cookbooks/barclamp/libraries/smartroles.rb

+      def include_recipe_smartly(*list_of_recipes)
+		# Iterate over the dependcies and  find if either have changed
+        # if it has changed include the recipe
+		#    "horizon::server" => [


Layout/Tab: Tab detected. (https://github.com/bbatsov/ruby-style-guide#spaces-indentation)

hound · 2019-02-25T13:05:34Z

chef/cookbooks/barclamp/libraries/barclamp_library.rb

+        end
+
+		# a attrlist is ['api','bind_port'] (array)
+		# and collection is assumed to be a hash like object


Layout/Tab: Tab detected. (https://github.com/bbatsov/ruby-style-guide#spaces-indentation)

hound · 2019-02-25T13:05:34Z

chef/cookbooks/barclamp/libraries/barclamp_library.rb

+          return prev_cfg, cur_cfg.default_attributes[barclamp]
+        end
+
+		# a attrlist is ['api','bind_port'] (array)


Layout/Tab: Tab detected. (https://github.com/bbatsov/ruby-style-guide#spaces-indentation)

hound · 2019-02-25T13:05:34Z

chef/cookbooks/barclamp/libraries/barclamp_library.rb

+		# this method will return collection['api']['bind_port']
+		# 
+		# - abort and return nil , if any key not found
+		# - log accordingly


Layout/Tab: Tab detected. (https://github.com/bbatsov/ruby-style-guide#spaces-indentation)
Layout/CommentIndentation: Incorrect indentation detected (column 2 instead of 8).

hound · 2019-02-25T13:05:34Z

chef/cookbooks/barclamp/libraries/barclamp_library.rb

+		# and collection is assumed to be a hash like object
+		# this method will return collection['api']['bind_port']
+		# 
+		# - abort and return nil , if any key not found


Layout/Tab: Tab detected. (https://github.com/bbatsov/ruby-style-guide#spaces-indentation)

hound · 2019-02-25T13:05:35Z

chef/cookbooks/barclamp/libraries/barclamp_library.rb

+		# a attrlist is ['api','bind_port'] (array)
+		# and collection is assumed to be a hash like object
+		# this method will return collection['api']['bind_port']
+		# 


Layout/Tab: Tab detected. (https://github.com/bbatsov/ruby-style-guide#spaces-indentation)
Layout/TrailingWhitespace: Trailing whitespace detected. (https://github.com/bbatsov/ruby-style-guide#no-trailing-whitespace)

hound bot reviewed Jan 9, 2019

View reviewed changes

nicolasbock suggested changes Jan 9, 2019

View reviewed changes

chef/cookbooks/barclamp/libraries/barclamp_library.rb Outdated Show resolved Hide resolved

hound bot reviewed Jan 10, 2019

View reviewed changes

chef/cookbooks/barclamp/libraries/smartroles.rb Show resolved Hide resolved

chef/cookbooks/barclamp/libraries/barclamp_library.rb Outdated Show resolved Hide resolved

hound bot reviewed Jan 10, 2019

View reviewed changes

chef/cookbooks/barclamp/libraries/smartroles.rb Show resolved Hide resolved

chef/cookbooks/barclamp/libraries/barclamp_library.rb Outdated Show resolved Hide resolved

sjamgade commented Jan 10, 2019

View reviewed changes

chef/cookbooks/barclamp/libraries/barclamp_library.rb Outdated Show resolved Hide resolved

sjamgade force-pushed the smartrecipes branch from 5edbeda to 319518b Compare January 22, 2019 14:35

hound bot reviewed Jan 22, 2019

View reviewed changes

sjamgade requested review from vuntz and rhafer January 22, 2019 15:12

hound bot reviewed Jan 22, 2019

View reviewed changes

sjamgade force-pushed the smartrecipes branch 2 times, most recently from e9f2fa9 to e8a34bf Compare January 22, 2019 15:40

skazi0 reviewed Jan 24, 2019

View reviewed changes

chef/cookbooks/barclamp/libraries/barclamp_library.rb Outdated Show resolved Hide resolved

skazi0 reviewed Jan 24, 2019

View reviewed changes

chef/cookbooks/barclamp/libraries/barclamp_library.rb Outdated Show resolved Hide resolved

sjamgade force-pushed the smartrecipes branch from e8a34bf to 51f94a3 Compare January 24, 2019 13:47

hound bot reviewed Jan 24, 2019

View reviewed changes

rhafer previously requested changes Jan 31, 2019

View reviewed changes

chef/cookbooks/barclamp/libraries/barclamp_library.rb Show resolved Hide resolved

rhafer requested a review from aspiers February 8, 2019 14:27

added commnets

5d04197

hound bot reviewed Feb 25, 2019

View reviewed changes

JanZerebecki added the wip label Jan 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added ability to include recipes smartly #1765

Added ability to include recipes smartly #1765

sjamgade commented Jan 9, 2019

hound bot Jan 9, 2019

nicolasbock left a comment

sjamgade commented Jan 9, 2019

nicolasbock commented Jan 9, 2019

sjamgade commented Jan 9, 2019 •

edited

Loading

hound bot Jan 22, 2019

hound bot Jan 22, 2019

skazi0 Jan 24, 2019

sjamgade Jan 24, 2019

skazi0 Jan 24, 2019 •

edited

Loading

sjamgade Jan 24, 2019

hound bot Jan 24, 2019

hound bot Jan 24, 2019

sjamgade commented Jan 24, 2019

vuntz commented Jan 24, 2019

sjamgade commented Jan 28, 2019

rhafer left a comment

sjamgade commented Jan 31, 2019

sjamgade commented Feb 8, 2019

rhafer commented Feb 8, 2019

hound bot Feb 25, 2019

hound bot Feb 25, 2019

hound bot Feb 25, 2019

hound bot Feb 25, 2019

hound bot Feb 25, 2019

hound bot Feb 25, 2019

hound bot Feb 25, 2019

hound bot Feb 25, 2019

hound bot Feb 25, 2019

hound bot Feb 25, 2019

Added ability to include recipes smartly #1765

Are you sure you want to change the base?

Added ability to include recipes smartly #1765

Conversation

sjamgade commented Jan 9, 2019

hound bot Jan 9, 2019

Choose a reason for hiding this comment

nicolasbock left a comment

Choose a reason for hiding this comment

sjamgade commented Jan 9, 2019

nicolasbock commented Jan 9, 2019

sjamgade commented Jan 9, 2019 • edited Loading

hound bot Jan 22, 2019

Choose a reason for hiding this comment

hound bot Jan 22, 2019

Choose a reason for hiding this comment

skazi0 Jan 24, 2019

Choose a reason for hiding this comment

sjamgade Jan 24, 2019

Choose a reason for hiding this comment

skazi0 Jan 24, 2019 • edited Loading

Choose a reason for hiding this comment

sjamgade Jan 24, 2019

Choose a reason for hiding this comment

hound bot Jan 24, 2019

Choose a reason for hiding this comment

hound bot Jan 24, 2019

Choose a reason for hiding this comment

sjamgade commented Jan 24, 2019

vuntz commented Jan 24, 2019

sjamgade commented Jan 28, 2019

rhafer left a comment

Choose a reason for hiding this comment

sjamgade commented Jan 31, 2019

sjamgade commented Feb 8, 2019

rhafer commented Feb 8, 2019

hound bot Feb 25, 2019

Choose a reason for hiding this comment

hound bot Feb 25, 2019

Choose a reason for hiding this comment

hound bot Feb 25, 2019

Choose a reason for hiding this comment

hound bot Feb 25, 2019

Choose a reason for hiding this comment

hound bot Feb 25, 2019

Choose a reason for hiding this comment

hound bot Feb 25, 2019

Choose a reason for hiding this comment

hound bot Feb 25, 2019

Choose a reason for hiding this comment

hound bot Feb 25, 2019

Choose a reason for hiding this comment

hound bot Feb 25, 2019

Choose a reason for hiding this comment

hound bot Feb 25, 2019

Choose a reason for hiding this comment

sjamgade commented Jan 9, 2019 •

edited

Loading

skazi0 Jan 24, 2019 •

edited

Loading